-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Store resources as components on singleton entities (v2) #21346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Store resources as components on singleton entities (v2) #21346
Conversation
This reverts commit 8c4269c.
the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh, this is exciting! I like the ResourceComponent<R: Resource>
approach! It looks good overall, and most of my comments are style nits.
I think the world_mut()
in the hooks is unsound, though, so we'll need to fix something there.
I'm also curious about the reasons for despawning the whole entity when replacing the resource or in resource_scope
. I had been expecting to preserve the entity and just add and remove the component, but I haven't thought through what the tradeoffs are between the two approaches.
Co-authored-by: Chris Russell <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exciting changes!
|
||
/// Type-erased equivalent of [`Components::valid_resource_id()`]. | ||
#[inline] | ||
#[deprecated( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- If I'm understanding this PR correctly, the behaviour of this function changes? So if I had a code
get_valid_resource_id(resource_type_id)
and upgraded bevy, the function call would start returning unexpected results, correct? In that case, I think it would be better to remove this function completely instead of just deprecating it (users will have to change the code anyways). - If I just have a
TypeId
and don't know the typeR
, is there still some way for me to check if it's registered? (for example, I could get the type IDs from iterating over the wholeTypeRegistry
registrations and checking forReflectResource
type data). If not, one option would be to add these function toReflectResource
.
(the same applies to get_resource_id
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're right, but I'll leave it to a clean-up PR. I really want to start wrapping this up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a pretty big footgun IMO and not something to clean up later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't kept up much with the changes from 0.17, so I did not do a deep dive review for this code. But at a glance, I don't see any major issues with the implementation.
My knowledge here is purely from my experimentation in my own ecs prototypes. At least, I think that's why my review was requested, so I'm reviewing more the concept than the code for now.
Bevy has kind of put itself in a difficult place because we present entities and resources fundamentally differently to users: Res
vs Single
, etc. From my research, an ideal world would have no distinction: a "resource" is just a component where it just so happens that there can only ever be one of them. Then, its some pretty thin utilities over the top, and you're done.
I haven't been following this feature super closely, but from what I can tell, that's not the direction Bevy wants to take–IIUC, mostly for compatibility reasons. That's why we have to make sure resources don't appear in queries, etc. That is, the hard part here is making sure that a resource entity doesn't happen to also have a Transform
component. In my prototype, I see no reason to disallow that, but I can understand the compatibility concerns for Bevy. (Ex: The Bevy way is to have a TargetedEnemy(Option<Entity>)
resource, not a ThisEntityIsTargeted
resource that also lives on the targeted entity.)
So the question is: how do we have it both ways? How can resources be on entities, but not be treated like entities? I think this pr (again, at a glance) does a pretty good job of walking that line.
My only real concern is that this might make it harder to create and use resources that do not correspond to rust types. That might not be an issues, but IIRC, it was supported before, and it's worth mentioning.
But I don't see anything "bad" about this approach in concept, given the compatibility concerns.
Hopefully that all made sense. There's a lot of moving parts here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I left some suggestions to expand on the tricky SAFETY
comments.
Co-authored-by: Chris Russell <[email protected]>
…evy into resource-as-components-v2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Goodness this is a big PR and quite hard to review, but I tried my best!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much better in terms of safety than the other PR. I still see a couple of nits, but nothing that's back-to-the-drawing-board level of blocking.
|
||
/// Type-erased equivalent of [`Components::valid_resource_id()`]. | ||
#[inline] | ||
#[deprecated( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a pretty big footgun IMO and not something to clean up later on.
I'm sorry for being so terse @SkiFire13, I'm really tired and annoyed from all the nitpicks and I only addressed the important comments. |
Important questions for follow-up here that I want a plan for before we merge:
|
|
With regards to number 2, after some discussion on Discord (thanks @chescock), we found the following system that should give conflicting access: fn system(
c: Query<&mut ResourceComponent<AssetChanges<Mesh>>, With<Internal>>,
r: Query<(), AssetChanged<Mesh>>) The query While I could write a long safety comment or input a couple of extra checks, I could also just remove resources from |
&& offending_entity != context.entity | ||
{ | ||
// the resource already exists and we need to overwrite it | ||
deferred_world.commands().entity(offending_entity).despawn(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think silently despawning an existing resource entity / replacing it is something we should allow as part of normal flows. This would break any existing references to the resource entity, including things like relationships. It would also erase any additional context we might add to the entity (ex: marker components). I personally think we are better off despawning the new entity, losing the new value, and logging an error. We lose many of the benefits of representing resources as entities if we can't rely on the stability of their entity id or their components.
(I am aware of how this behavior would interact with scenes, see my other comment)
// - World accesses follow the registered accesses. | ||
unsafe impl<'a, T: Resource> SystemParam for Res<'a, T> { | ||
type State = ComponentId; | ||
type State = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using QueryState here feels very "heavy" for what we're doing here (QueryState is a "large" type, maintaining the extra access sets takes work, the query lifecycle has overhead to keep the matching archetype list in place, we don't need cached archetype lists here as we aren't iterating, etc). In practice, it seems like we really just need to orchestrate ReadFetch, which is considerably smaller / simpler / less expensive.
}); | ||
// The default component hook behavior for adding an already existing reasource is to replace it. | ||
// We don't want that here. | ||
if self.contains_resource::<R>() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels inefficient / we're doing double lookups. Why not just if let Some(id) = self.resource_id::<R>() { return id }
?
// First ensure that every entity in the scene has a corresponding world | ||
// entity in the entity map. | ||
for scene_entity in &self.entities { | ||
for scene_entity in self.entities.iter().chain(self.resources.iter()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spawning resources as "normal" entities means that we're relying on the "create new resource entity and let hooks despawn the previous resource entity if it exists" behavior, which imo needs to go (see my previous comment).
I'd rather special case resources in scenes than live in the "resource entities are unstable" world.
score: 1, | ||
4294967299: ( | ||
components: { | ||
"bevy_ecs::resource::ResourceComponent<scene::ResourceA>": (( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is worth pointing out that from a user-facing perspective, this is strictly worse UX by a pretty wide margin. Harder to read, harder to write, harder to understand, takes up more space, error prone (any additional data you attach here will be replaced the second another scene tries to set the resource). Special casing resources in scenes makes a lot of sense I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed: ideally we can avoid any breakage to the representation of resources in scenes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That might be possible, but I would still like to support the entity notation. If / when people start attaching extra data to resource entities, that should be serializable / deserializable through scenes. The only solution I can think of is to store resources twice (both in entities and in resources). It would look something like this:
// valid
resources: {
"scene::ResourceA": (
score: 1,
),
},
},
entities: {
... // this does not contain a ResourceComponent<ResourceA> entity
}
// also valid
resources: {
... // this does not contain a ResourceA resource
},
entities: {
4294967291: (
components: {
"bevy_ecs::entity_disabling::Internal": (),
"bevy_ecs::resource::IsResource": (),
"bevy_ecs::resource::ResourceComponent<scene::ResourceA>": ((
score: 1,
)),
"replicon::Replicate": (), // useful additional components that wouldn't fit in `resources { ... }`
},
),
}
// this would be invalid
resources: {
"scene::ResourceA": (
score: 1,
),
},
},
entities: {
4294967291: (
components: {
"bevy_ecs::entity_disabling::Internal": (),
"bevy_ecs::resource::IsResource": (),
"bevy_ecs::resource::ResourceComponent<scene::ResourceA>": ((
score: 1,
)),
"replicon::Replicate": (),
},
),
}
What do you think of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you reconcile the entity-notation version with this comment?
"scene::ResourceA": ( | ||
score: 1, | ||
4294967299: ( | ||
components: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the motivators for this change is that we get "free" functionality for resources, in the context of things like entity inspectors. However I think this is a great example of why that "free" functionality would require additional custom handling for resources to make it "good".
An entity inspector would (logically) display this as something like:
entity_id: 11v0,
components: [
ResourceComponent<ResourceA>(ResourceA {
score: 1
}),
IsResource
Internal
]
And notably this would be hidden by default because it is Internal.
Is this actually better than having a separate resource section of the inspector that displays it as:
ResourceA {
score: 1
}
without needing to disable the Internal filter and manually filter down to IsResource?
Of course, you could build that functionality on top of the entity representation. But the ideal UX is very different from what we get by default, and the default UX doesn't really win us much in the majority of cases. That is also true for the scene case (see my other comment).
#[derive(Component)] | ||
#[require(IsResource, Internal)] | ||
#[component(on_add = on_add_hook, on_remove = on_remove_hook)] | ||
#[repr(transparent)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not in love with the ResourceComponent wrapper. Why not implement Component directly for the resource? This would make it appear nicely (without a wrapper type) in scenes / brp / inspectors, it would make the component type id match the type of the resource, it would make it look nicer in queries, easier to consume when being queried for, would result in less codegen, fewer reflected types, etc, etc, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was previously attempted, and caused problems with type safety and open questions around overlap between ReflectResource and ReflectComponent. I'm open to reverting back to that design: I didn't consider the codegen concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you prefer an approach without the wrapper, you can review the first version (#20934).
@@ -0,0 +1,20 @@ | |||
--- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm struggling to see this as a net win for users (or maintainers). I can see a world where we shore things up and the calculus changes, but in its current form I don't feel compelled to merge. The cost is too high and the benefits too intangible.
The Pros
- We get "free" access to entity/component features, such as:
- Hooks: not really a benefit as the add/remove hooks are already "occupied"
- Component lifecycle observers: these are nice! but we could easily add equivalent events for resources, and I suspect using these events will be niche.
- Ability to attach additional components/ data to resources + query for them: I'm not yet sure why we'd want this. I'm guessing we could find use cases, but I suspect they will be niche.
- Ability to "target" resources with relationships. I'm not sure why we'd want this. I suspect someone could find a use case, but it is probably niche.
- We get "free" support for resources in tooling like scenes, entity inspectors, and BRP. I don't see this as a particularly big win in practice, given the simplicity of building the custom paths for resources, and the default behaviors often have downsides / are suboptimal (see my other comments)
The Cons
- Resource access is now considerably more expensive and convoluted internally (see my other comments)
- The current design relies on resource entities being unstable. This loses us a lot of the wins of the entity representation (see my other comments).
- Many of the "free" entity / component behaviors are wrong / suboptimal when compared to a custom resource path: entity inspectors, scenes, spawning entities containing the resource component, etc (see my other comments).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ability to attach additional components/ data to resources + query for them: I'm not yet sure why we'd want this. I'm guessing we could find use cases, but I suspect they will be niche.
This might be different for Bevy since you already get rust traits to configure behavior, but in the case of flecs it's very common to do this: https://www.flecs.dev/flecs/md_docs_2ComponentTraits.html (a trait in flecs is a component added to a component).
It's also used to put components under module (plugin) parents to mirror the type hierarchy, to store component names, reflection data, used by Flecs users to add custom attributes etc.
None of these things need to be stored on a component entity, but once they are it's a nice & consistent way for users to interact with the API :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ability to attach additional components/ data to resources + query for them: I'm not yet sure why we'd want this. I'm guessing we could find use cases, but I suspect they will be niche.
This is required for networking by both replicon/lightyear.
Replication resources would involve adding a component such as Replicate
on the Resource entity (+ other components to customize replication).
It also very useful to be able to write replication logic once for entity/components and get resource replication for free, as opposed to having to special-case resources.
I could implement it by rolling out my own singleton entity that points to that resource, but it's probably be easier to simply have resources be entities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see a world where we shore things up and the calculus changes,
What would that world look like? Can I tweak the current PR and get it to a state in which you'd agree to merge it, or is it dependent on other efforts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised to see questioning of the utility of resources-as-entities in general. Maybe I'm over-indexing into it, but I feel that some way to express resources-as-entities is really important! I hope you don't mind me adding a couple more real-world motivating examples.
Singleton components
There are many instances of singleton components in my code bases. As I'm developing my web framework, there are a few obvious targets: namely, HTML elements and browser resources that only exist once.
- The browser window and document (among other items)
- The document's
<html>
,<head>
, and<body>
elements.
These are currently expressed in my code as components. After all, all of these objects are event targets. I can add an event handler with my Events
relationship. And it probably goes without saying that the HTML elements need to exist in the element hierarchy to function properly in my framework.
Consequently, my code base is littered with Single<&Window>
params that would be more elegantly expressed as Res<Window>
.
The same idea also comes up with items under my control, like a right-click context menu element. There should only ever be at most one ContextMenu
component, but it would benefit from relationships, additional components, and lifecycle observers if I want to render it to HTML.
Resource-dependent entities
In bevy_seedling
I have an index resource for dynamically-generated sampler pools. (Sampler pools are entities with a unique label.) Currently, this resource is private. This is partly because my crate's API would otherwise expose a correctness trap; removing this resource could leave behind unreachable pools.
If resources were entities, I could make these pools children of the index resource. Despawning the index would therefore necessarily clean up any indexed pools. I could similarly provide a Replace
observer to catch that operation. I could then expand on the functionality of this index by using its presence to dictate whether dynamic pools are created in the first place.
Overall, an entity-integrated API would be clearer, simpler, and more correct.
It's true these APIs are largely expressible one way or another without resources-as-entities. And I can understand resistance to the implementation of the PR. But to me, there's a lot of immediate value in the idea itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of these things need to be stored on a component entity, but once they are it's a nice & consistent way for users to interact with the API :)
Yup I agree that being able to add additional behaviors / metadata to resources is nice.
This is required for networking by both replicon/lightyear.
I see the appeal!
What would that world look like? Can I tweak the current PR and get it to a state in which you'd agree to merge it, or is it dependent on other efforts?
It is dependent on resolving the issues I have outlined in my comments. If you can do that in this PR, thats fine by me.
I'm surprised to see questioning of the utility of resources-as-entities in general
I'm not questioning whether or not the utility exists. I'm only making a relative-value judgement: I don't see the utility as high enough to compromise on the concerns I have outlined in my review.
Consequently, my code base is littered with Single<&Window> params that would be more elegantly expressed as Res.
Bevy's Window
component is not "unique", as bevy supports multiple windows. I would probably oppose a derive(Resource)
on Window
.
Resource-dependent entities
I see the appeal!
Yep, this is out of scope and mostly orthogonal. Happy to elaborate on this a bit, but I don't want to derail. Here's a more in-depth explanation (click me).Non-Send PlansThere's a lot of technical debt in our scheduling that we need to very carefully clean up. Frame-pacing, input timestamps, a decoupled rendering loop, these are all closely related, and nonsend data access is a big part of the tangle. One potentially important observation is that may examples of non-send data are fully locked to the main thread. This is especially true on web. In my opinion, we can exploit this. I view our next move as moving the ecs run loop to it's own thread, while keep all non-send data on the main thread. We will access main-thread data via an escape hatch, that lets you spawn tasks from the ecs to be executed within the main-thread winit event loop. Here's a rough sketch: // Events sent from the winit thread to the ecs thread
enum RuntimeEvent {
Resumed,
WindowEvent {
timestamp: Instant,
window_id: WindowId,
event: WindowEvent
},
...
}
// Events sent from the ecs (or anywhere else) to execute on the winit thread
struct WinitTask { ... }
// The winit event loop handler
struct WinitApp {
pub runtime_events: Sender<WindowEvent>
pub attachments: AnyMap // storage for arbitrary non-send data
}
// A winit handler that forwards basically everything to another thread
impl ApplicationHandler<WinitTask> for WinitApp {
fn resumed(&mut self, event_loop: &ActiveEventLoop) {
self.runtime_events.send(RuntimeEvent::Resumed);
}
fn window_event(
&mut self,
event_loop: &ActiveEventLoop,
window_id: WindowId,
event: WindowEvent,
) {
// Grab timestamps when receiving winit timestamps
let timestamp = Instant::now();
self.runtime_events.send(RuntimeEvent::WindowEvent { timestamp, window_id, event });
}
...
// Executes tasks sent via `EventLoopProxy::send_event`
fn user_event(&mut self, event_loop: &ActiveEventLoop, event: WinitTask) {
event.execute();
}
}
// Runs the ECS event loop
fn ecs_runtime(
mut world: World,
mut events: Receiver<RuntimeEvent>,
) {
let mut start_time = Instant::now();
loop {
// Handle events while waiting to start the schedule
while let Ok(event) = events.recv_deadline(start_instant) {
handle_event(&mut world, event);
}
// Execute a single "tick" of the world
execute_schedules(&mut world);
// Use frame-pacing to determine when to start the next frame
start_time = next_frame_start();
}
}
// Holds the proxy event loop so main-thread tasks can be queued from anywhere
static WINIT_TASK_SENDER: OnceLock<EventLoopProxy<WinitTask>> = OnceLock::new();
// Executes the provided function within the winit event loop
fn within_winit<F>(func: F)
where
F: Fn(&mut WinitApp, &mut event_loop: &ActiveEventLoop) + Send + 'static
{
let Some(task_sender) = WINIT_TASK_SENDER.get().expect("winit not enabled");
let task = WinitTask::new(func);
task_sender.send_event(task).expect("failed to send data to winit");
}
// An imaginary main function
fn main() {
let event_loop = EventLoop::<WinitTask>::with_user_event().build().unwrap();
event_loop.set_control_flow(ControlFlow::Wait);
let event_loop_proxy = event_loop.create_proxy();
MAIN_THREAD.set(true);
WINIT_TASK_SENDER.set(event_loop_proxy);
let (runtime_events_sender, runtime_events_receiver) = unbounded();
let world = World::new();
let runtime_thread = thread::spawn(move || {
ecs_runtime(world, runtime_events_receiver)
})
let mut app = WinitApp {
runtime_events: runtime_events_sender
};
event_loop.run_app(&mut app);
}
// An example system
fn my_system() {
// To run some work on the main thread, just call `within_winit`
within_winit(|app, event_loop| {
// The winit app lets you access arbitrary nonsend data
let mut nonsend_data = app.attachments.get_mut::<MyNonSendData>().unwrap();
nonsend_data.foo();
})
} In my view, this work is pretty well isolated, and as more to do with |
This is largely identical to #20934, except we store resource data on a
ResourceComponent<R: Resource>
component on a singleton entity. This has the benefit of sidestepping the annoying double-derive problem of having to both deriveComponent
andResource
for each resource.For more information check out the original PR description #20934.
Closes #20934, #19711, #17485 and is part of #19731.
Other Changes
Deprecate
Components
methodsThis PR deprecates
Components::get_valid_resource_id(type_id: TypeId)
, andComponents::get_resource_id(type_id: TypeId)
. This is because resources (excluding non-send) are not registered with theirTypeId
anymore. Instead they're registered by theTypeId
ofResourceComponent<SomeResource>
. This changes the API as follows:becomes
This becomes confusing, as the method name suggests we're dealing with resources, while in order to use it correctly, we must already be aware that resources are actually hidden behind a component. The same is true for the
get_valid_resource_id
method. Both are being deprecated in favour of eitherresource_id
andvalid_resource_id
for when the type is available, orget_id
orget_valid_id
when it isn't. Using the latter two does require the user to wrap the type inResourceComponent<_>
in order for it to work correctly.Registering a Resource Type
When registering a resource type manually through
app.register_type::<R>()
or through theAppTypeRegistry
, a user must wrap the type inResourceComponent<R>
in order for it to be properly registered.However, when using the untyped API's like
get_resource_by_id
, the user can simplyread::<R>()
sinceResourceComponent<_>
is transparent.Resources implement
MapEntities
by defaultIn order to have
MapEntities
working with resources,#[derive(Resource)]
now automatically implementsMapEntities
. This makes it such thatno longer compiles. Instead write: